-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Addding support for inverted tiles in TileGrid #10102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
dhalbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IThe use of a tuple for the coordinates seems unusual to me. The constructor takes x and y arguments. Are there other displayio things that take tuples for (x, y)?
tilegrid.contains()
contains() and used a tuple to make it consistent with adafruit_button.contains() which it basically implements for TileGrids to let them act as touch buttons. I believe adafruit_button.contains() accepted a tuple in order to be compatible with values coming out of touch display drivers.
I actually did try using x and y arguments at first though, and I'd be happy to get back to that. I failed at figuring out how to properly do something like |
dhalbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks for the explanation.
build failures, looks like due to size
|
I've got a few ideas in mind that might be able to trim some bits of code. I'll see if I can get it under the limit for the one that overflowed. |
|
I was able to shave 30 or so bytes by refactoring the nested for loops, and the larger branching if statements. However it's still not enough to fit the feather m0 supersized. I've disabled If someone can point me towards how to use |
|
If you think using the tuple is more consistent with the rest of the API, then I'd say stick with that. There are a bunch of uses of |
|
okay, I'll leave it alone for now. it does also match the tile getter and setter that use square bracket access in python code like |
|
This needs to handle dirty area inside of set_inverted. Currently the change wont become visible unless something else changes causing the area of the tile to get refreshed. In the example script above it works because it gets set before being rendered the first time I think. But with an added time.sleep() and then changing some invert values they do not take effect currently. |
|
The latest commit resolves that issue by forcing refresh on tiles when their inverted state changes. This new example code validates that it behaves as expected: |
|
This needed validation on the x,y values in order to avoid hard faulting if the user passes values outside the range of the tilegrid size. After adding those it pushed the feather_m0_supersized over the limit again. I ended up disabling displayio on that device which leaves plenty of room. I am open to suggestion if there is something else that would be better to disable on that device. |
tannewt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking this on! Unfortunately, I don't think this is the right approach.
Instead, I think it'd be better to make a terminal-specific pixel_shader. Tilegrid passes in the tile coordinates to the pixel shader. The shader can then store the foreground and background color indices for each tile.
This would then leave TileGrid nearly unchanged and not cost more memory for inverts on every TG. It'd also allow recoloring of foreground and background of each character.
What do you think?
| void common_hal_displayio_tilegrid_set_inverted(displayio_tilegrid_t *self, uint16_t x, uint16_t y, bool inverted) { | ||
| uint16_t tile_location = y * self->width_in_tiles + x; | ||
| self->inverts[tile_location] = inverted; | ||
| common_hal_displayio_tilegrid_set_tile(self, x, y, common_hal_displayio_tilegrid_get_tile(self, x, y)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather you factor out the code to mark a tile area dirty so you can use it here. That way we can optimize set_tile to skip a dirty area when the tile index is set to the current value.
| if (common_hal_displayio_palette_get_len(self->pixel_shader) == 2) { | ||
| if (self->inverts[tile_location]) { | ||
| input_pixel.pixel = (input_pixel.pixel + 1) % 2; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about this? It'd do something for all palette sizes.
| if (common_hal_displayio_palette_get_len(self->pixel_shader) == 2) { | |
| if (self->inverts[tile_location]) { | |
| input_pixel.pixel = (input_pixel.pixel + 1) % 2; | |
| } | |
| } | |
| if (self->inverts[tile_location]) { | |
| input_pixel.pixel = common_hal_displayio_palette_get_len(self->pixel_shader) - 1 - input_pixel.pixel; | |
| } |
| mp_obj_t pixel_shader, uint16_t width, uint16_t height, | ||
| uint16_t tile_width, uint16_t tile_height, uint16_t x, uint16_t y, uint8_t default_tile) { | ||
| uint32_t total_tiles = width * height; | ||
| self->inverts = (bool *)m_malloc(total_tiles); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will allocate one byte per tile. It may save some code size to do so but it'll leave 7 unused bits.
| } | ||
|
|
||
| bool common_hal_displayio_tilegrid_get_inverted(displayio_tilegrid_t *self, uint16_t x, uint16_t y) { | ||
| uint16_t tile_location = y * self->width_in_tiles + x; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add:
if (self->inverts == NULL) {
return false;
}
| .top_left_x = {0}, | ||
| .top_left_y = {0}, | ||
| .tiles = 0, | ||
| .inverts = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .inverts = false, | |
| .inverts = NULL, |
|
Will make a new branch and PR for this with the |
Tested successfully with this on PyPortal Titano:
which shows several glyphs from the built-in font and inverts one of them using this new feature.
Also note that this supports only TileGrids which use Palettes that have 2 colors. It is ultimately intended for use along with terminalio.Terminal for cursor visuals within the terminal.